Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fs: filehandle.read(buffer) can't read file when options are omitted #51087

Merged
merged 3 commits into from
Dec 22, 2023
Merged

fs: filehandle.read(buffer) can't read file when options are omitted #51087

merged 3 commits into from
Dec 22, 2023

Conversation

pulkit-30
Copy link
Contributor

fix: #47183

  • make length arg in fh.read(buffer, offset, position, length) default to length ?? buffer.byteLength - offset

Code

import fs from "fs/promises";

async function test() {
  let path = './ex.txt';

  let buf = Buffer.alloc(4);
  let handle;
  try {
    handle = await fs.open(path);
    {
      let buf = Buffer.alloc(4);
      let { bytesRead, buffer } = await handle.read(buf);
      console.log(bytesRead);
      console.log(buffer);
      console.log(buf);
    }
  } finally {
    await handle?.close();
  }
}
test();

before

0
<Buffer 00 00 00 00>
<Buffer 00 00 00 00>

now

4
<Buffer 48 65 6c 6c>
<Buffer 48 65 6c 6c>

That's my second attempt at this issue 😅

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run. labels Dec 7, 2023
bricss
bricss reviewed Dec 7, 2023
View reviewed changes
lib/internal/fs/promises.js Outdated Show resolved Hide resolved
@pulkit-30 pulkit-30 requested a review from bricss December 8, 2023 18:27
@H4ad H4ad added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. request-ci Add this label to start a Jenkins CI on a PR. labels Dec 12, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 12, 2023
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56248/

BridgeAR
BridgeAR approved these changes Dec 12, 2023
View reviewed changes
Copy link
Member

@BridgeAR BridgeAR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM (while we could remove the length assignment in line 651 and 662)

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56284/

@bricss bricss mentioned this pull request Dec 17, 2023
Merged
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/56391/

@jasnell jasnell added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2023
@nodejs-github-bot nodejs-github-bot added commit-queue-failed An error occurred while landing this pull request using GitHub Actions. and removed commit-queue Add this label to land a pull request using GitHub Actions. labels Dec 22, 2023
@nodejs-github-bot
Copy link
Collaborator

Commit Queue failed
- Loading data for nodejs/node/pull/51087
✔  Done loading data for nodejs/node/pull/51087
----------------------------------- PR info ------------------------------------
Title      fs: filehandle.read(buffer) can't read file when options are omitted (#51087)
Author     Pulkit Gupta  (@pulkit-30)
Branch     pulkit-30:fix-47183 -> nodejs:main
Labels     fs, author ready, needs-ci
Commits    3
 - fs: make offset, position & length args in fh.read() optional
 - fix test
 - fix suggestions
Committers 1
 - pulkit-30 
PR-URL: https://github.com/nodejs/node/pull/51087
Fixes: https://github.com/nodejs/node/issues/47183
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/51087
Fixes: https://github.com/nodejs/node/issues/47183
Reviewed-By: Benjamin Gruenbaum 
Reviewed-By: Vinícius Lourenço Claro Cardoso 
Reviewed-By: Ruben Bridgewater 
Reviewed-By: Luigi Pinca 
Reviewed-By: James M Snell 
--------------------------------------------------------------------------------
   ℹ  This PR was created on Thu, 07 Dec 2023 16:55:28 GMT
   ✔  Approvals: 5
   ✔  - Benjamin Gruenbaum (@benjamingr) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1774088173
   ✔  - Vinícius Lourenço Claro Cardoso (@H4ad): https://github.com/nodejs/node/pull/51087#pullrequestreview-1777399313
   ✔  - Ruben Bridgewater (@BridgeAR) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1777519295
   ✔  - Luigi Pinca (@lpinca): https://github.com/nodejs/node/pull/51087#pullrequestreview-1780448292
   ✔  - James M Snell (@jasnell) (TSC): https://github.com/nodejs/node/pull/51087#pullrequestreview-1793960251
   ✔  Last GitHub CI successful
   ℹ  Last Full PR CI on 2023-12-19T17:04:36Z: https://ci.nodejs.org/job/node-test-pull-request/56391/
- Querying data for job/node-test-pull-request/56391/
   ✔  Last Jenkins CI successful
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/main up to date...
From https://github.com/nodejs/node
 * branch                  main       -> FETCH_HEAD
✔  origin/main is now up-to-date
- Downloading patch for 51087
From https://github.com/nodejs/node
 * branch                  refs/pull/51087/merge -> FETCH_HEAD
✔  Fetched commits as 4f3261f10183..1de21b873667
--------------------------------------------------------------------------------
Auto-merging doc/api/fs.md
[main fd0e346463] fs: make offset, position & length args in fh.read() optional
 Author: pulkit-30 
 Date: Thu Dec 7 22:16:46 2023 +0530
 3 files changed, 23 insertions(+), 3 deletions(-)
[main a0f036d617] fix test
 Author: pulkit-30 
 Date: Thu Dec 7 22:17:44 2023 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
[main dfb1cf7171] fix suggestions
 Author: pulkit-30 
 Date: Fri Dec 8 00:38:07 2023 +0530
 1 file changed, 1 insertion(+), 1 deletion(-)
   ✔  Patches applied
There are 3 commits in the PR. Attempting autorebase.
Rebasing (2/6)

Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fs: make offset, position & length args in fh.read() optional


PR-URL: #51087

Fixes: #47183

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Vinícius Lourenço Claro Cardoso contact@viniciusl.com.br

Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

Reviewed-By: Luigi Pinca luigipinca@gmail.com

Reviewed-By: James M Snell jasnell@gmail.com


[detached HEAD 7932ec3bda] fs: make offset, position & length args in fh.read() optional

Author: pulkit-30 pulkit30.bsr@gmail.com

Date: Thu Dec 7 22:16:46 2023 +0530

3 files changed, 23 insertions(+), 3 deletions(-)

Rebasing (3/6)

Rebasing (4/6)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fix test


PR-URL: #51087

Fixes: #47183

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Vinícius Lourenço Claro Cardoso contact@viniciusl.com.br

Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

Reviewed-By: Luigi Pinca luigipinca@gmail.com

Reviewed-By: James M Snell jasnell@gmail.com


[detached HEAD 224a463637] fix test

Author: pulkit-30 pulkit30.bsr@gmail.com

Date: Thu Dec 7 22:17:44 2023 +0530

1 file changed, 1 insertion(+), 1 deletion(-)

Rebasing (5/6)

Rebasing (6/6)


Executing: git node land --amend --yes

--------------------------------- New Message ----------------------------------

fix suggestions


PR-URL: #51087

Fixes: #47183

Reviewed-By: Benjamin Gruenbaum benjamingr@gmail.com

Reviewed-By: Vinícius Lourenço Claro Cardoso contact@viniciusl.com.br

Reviewed-By: Ruben Bridgewater ruben@bridgewater.de

Reviewed-By: Luigi Pinca luigipinca@gmail.com

Reviewed-By: James M Snell jasnell@gmail.com


[detached HEAD c191ac4bbb] fix suggestions

Author: pulkit-30 pulkit30.bsr@gmail.com

Date: Fri Dec 8 00:38:07 2023 +0530

1 file changed, 1 insertion(+), 1 deletion(-)


Successfully rebased and updated refs/heads/main.


ℹ  Add commit-queue-squash label to land the PR as one commit, or commit-queue-rebase to land as separate commits.
https://github.com/nodejs/node/actions/runs/7296297391

@H4ad H4ad added commit-queue Add this label to land a pull request using GitHub Actions. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. and removed commit-queue-failed An error occurred while landing this pull request using GitHub Actions. labels Dec 22, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 22, 2023
@nodejs-github-bot nodejs-github-bot merged commit 48cdb88 into nodejs:main Dec 22, 2023
66 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 48cdb88

RafaelGSS pushed a commit that referenced this pull request Jan 2, 2024
@pulkit-30 @RafaelGSS
fs: make offset, position & length args in fh.read() optional
e5a8fa0 
PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2024
Merged
@markrian markrian mentioned this pull request Jan 2, 2024
Closed
richardlau pushed a commit that referenced this pull request Mar 25, 2024
@pulkit-30 @richardlau
fs: make offset, position & length args in fh.read() optional
a8fd01a 
PR-URL: #51087
Fixes: #47183
Reviewed-By: Benjamin Gruenbaum <benjamingr@gmail.com>
Reviewed-By: Vinícius Lourenço Claro Cardoso <contact@viniciusl.com.br>
Reviewed-By: Ruben Bridgewater <ruben@bridgewater.de>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Mar 25, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@H4ad H4ad H4ad approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@lpinca lpinca lpinca approved these changes

@benjamingr benjamingr benjamingr approved these changes

@jasnell jasnell jasnell approved these changes

@bricss bricss Awaiting requested review from bricss

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. fs Issues and PRs related to the fs subsystem / file system. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

filehandle.read(buffer) can't read file when options are omitted
8 participants
@pulkit-30 @nodejs-github-bot @jasnell @benjamingr @lpinca @bricss @BridgeAR @H4ad